Update proxy features #45979
Merged
Update proxy features #45979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
7ef795d
to
b610f4c
Compare
zmb3
reviewed
Aug 28, 2024
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
a5561d3
to
0b30b7b
Compare
…tirola/update-proxy-features
…tirola/update-proxy-features
…tirola/update-proxy-features
mcbattirola
force-pushed
the
mcbattirola/update-proxy-features
branch
from
08a8cf0
to
5c8233e
Compare
…tirola/update-proxy-features
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
mcbattirola
added
the
no-changelog
rosstimothy
reviewed
Sep 16, 2024
lib/web/features.go
Outdated
Comment on lines
26
to
27
| // SetClusterFeatures sets the flags for supported and unsupported features | ||
| func (h *Handler) SetClusterFeatures(features proto.Features) { |
There was a problem hiding this comment.
Should this be an unexported function? What external entities should be capable of temporarily overwriting the last stored features?
Suggested change
| // SetClusterFeatures sets the flags for supported and unsupported features | |
| func (h *Handler) SetClusterFeatures(features proto.Features) { | |
| // setClusterFeatures sets the flags for supported and unsupported features | |
| func (h *Handler) setClusterFeatures(features proto.Features) { |
There was a problem hiding this comment.
Ideally, but there is a test in e/lib/web/plugindescriptor_okta_test.go: that is using it, and updating this test is not trivial.
I've added a TODO to change this method to unexported, and I'll sync with someone more familiar with those tests to change it.
rosstimothy
approved these changes
Sep 30, 2024
public-teleport-github-review-bot
bot
removed request for
r0mant,
ryanclark,
kiosion and
rudream
…ational/teleport into mcbattirola/update-proxy-features
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…tirola/update-proxy-features
|
@mcbattirola See the table below for backport results.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The proxy service only reads features at startup, leading to potential stale features if they change after the initial load. Some code paths (e.g., SCIM web handlers) check features during requests, which means proxies must be restarted when features change.
This PR introduces a
startFeatureWatchermethod in the web handler. The watcher periodically pings the auth server (every 5 to 10 minutes) to fetch and update features, ensuring they remain up-to-date without restarting the proxy.I chose to add the watcher within the handler rather than
service.TeleportProcessto minimize the impact on other services. This avoids unintended side effects, as the proxy handler is currently the only component requiring dynamic feature updates. If other services need this in the future, we can consider moving the watcher to a higher-level component. Let me know what you think.Closes #39161